Repository navigation
Add support for sending header values to Quepid - #482
matt-bernhardt wants to merge 2 commits into
Conversation
Coverage Report for CI Build 41711839038Coverage increased (+0.001%) to 98.424%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
❌ 5 blocking issues (5 total)
@qltysh one-click actions:
|
| quepid_ua = 'Quepid/1.0 (Web Scraper)' | ||
| ClimateControl.modify FORMAT_TOKEN: secret_value do | ||
| mock_timdex_search_with_hits(10) | ||
| get "/results?q=test&format=json", headers: { 'HTTP_USER_AGENT' => quepid_ua, 'HTTP_FORMAT_TOKEN' => secret_value } |
049af78 to
5cda744
Compare
** Why are these changes being introduced: We want to allow Quepid to provide the format token via a request header, rather than via the query parameters. ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/use-660 ** How does this address that need: This updates the search controller to check either the params or the request header for the format token, preferring the headers first. ** Document any side effects to this change: This adds pretty minimal test coverage for the request header for now, but these tests will be re-written more heavily when we go back through and remove support for query parameters (assuming we take that step - it might be good to keep that support around for browser-based debugging)
5cda744 to
ea3dda0
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Blank header tokens can cause server errors instead of 401 responses, while valid header tokens fail to preserve the existing throttle bypass.
2 open issues
What changed in this PR
Adds header-based token authentication for Quepid’s JSON search requests while retaining query-parameter support.
Changes:
- Accepts
Format-Tokenheaders, preferring nonblank header values over query parameters. - Adds tests for successful header authentication and rejection of invalid tokens or missing server configuration.
| File | Description |
|---|---|
test/controllers/search_controller_test.rb |
Adds header-token authentication coverage alongside existing parameter tests. |
app/controllers/search_controller.rb |
Reads and validates tokens from headers or query parameters. |
Review effort: Balanced
| return false unless ENV.fetch('FORMAT_TOKEN', '').present? | ||
|
|
||
| false | ||
| request.headers.key?('HTTP_FORMAT_TOKEN') || params.key?(:format_token) |
There was a problem hiding this comment.
I've applied the change to the search controller to the rack attack configuration, and updated the tests for the format token bypass to confirm that both options are still valid (as well as what the precedence is should tokens arrive via both pathways).
| # a boolean. | ||
| def valid_token? | ||
| return true if ActiveSupport::SecurityUtils.secure_compare(params[:format_token], ENV.fetch('FORMAT_TOKEN', '')) | ||
| candidate = request.headers['HTTP_FORMAT_TOKEN'].presence || params[:format_token] |
There was a problem hiding this comment.
I've updated the search controller in a different way, using .presence within the format_tokens_defined? method as well as within the validity check. This prevents the need for a guard clause on candidate.blank?, as it stops valid_token? from being called in the first place. I've added a test on lines 2001-2013 to confirm the intended behavior (red-green refactor FTW, I think)
Changes here are: - protect against blank format_tokens - extend support for request headers to rack attack integration - add tests for both rack attack and controller to clarify precedence between the header and params (header should govern if both are supplied)
|
|
||
| ClimateControl.modify(FORMAT_TOKEN: secret_value, FEATURE_BOT_DETECTION: 'true') do | ||
| mock_timdex_search_with_hits(10) | ||
| get "/results?q=test&format=json", headers: { 'HTTP_FORMAT_TOKEN' => secret_value, 'HTTP_USER_AGENT' => quepid_ua } |

This adds support for specifying the format token via the request header, in addition to via a querystring parameter. This is an addition, rather than a replacement, because at the moment Quepid is configured to use the querystring option - and I'd like not to break that via a hard cutover.
I'm not sure right now whether we'll want to follow this up with a PR to remove the querystring option (once Quepid has been reconfigured). There may be value in continuing to use the querystring for local or in-browser use.
Ticket: https://mitlibraries.atlassian.net/browse/USE-660
To confirm this integration: I've updated the experimental search endpoint within our Quepid instance to point at this review app, specifying the format token via a request header. You should be able to run the mapper wizard within Quepid and confirm that this integration now works as intended.
Developer
Accessibility
New ENV
Approval beyond code review
Additional context needed to review
E.g., if the PR includes updated dependencies and/or data
migration, or how to confirm the feature is working.
Code Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing